Skip to content

PARQUET-243: Add Avro reflect support#165

Closed
rdblue wants to merge 8 commits into
apache:masterfrom
rdblue:PARQUET-243-add-avro-reflect
Closed

PARQUET-243: Add Avro reflect support#165
rdblue wants to merge 8 commits into
apache:masterfrom
rdblue:PARQUET-243-add-avro-reflect

Conversation

@rdblue

@rdblue rdblue commented Apr 7, 2015

Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a bit of documentation to explain the salient differences between this class and AvroRecordConverter? It's quite hard to tell just by looking.

@tomwhite

Copy link
Copy Markdown
Member

This looks great.

I think AvroParquetOutputFormat needs to be parameterized by T (like AvroParquetInputFormat already is). Also can you add a TestReflectInputOutputFormat to test the formats when using reflect.

@rdblue rdblue force-pushed the PARQUET-243-add-avro-reflect branch from d526347 to 77691a0 Compare April 30, 2015 01:34
@rdblue rdblue force-pushed the PARQUET-243-add-avro-reflect branch from 77691a0 to a1a17b4 Compare May 14, 2015 17:21
@rdblue

rdblue commented May 14, 2015

Copy link
Copy Markdown
Contributor Author

@tomwhite, I've updated this and rebased it after the rename.

I didn't need to duplicate the read or write classes where the type bound has changed because they are extending classes or interfaces with a weaker type bound. In that case, the JVM writes the method using the parent's descriptor, so they are binary compatible. I also verified this expected behavior locally with test classes, by compiling against <T extends Something> then changing to just <T> and verifying that the dependency binaries are interchangeable. In addition, I made sure that without the superclass or superinterface, binary compatibility is indeed broken.

I think this is ready to go. Thanks for taking a look!

@tomwhite

Copy link
Copy Markdown
Member

Thanks for checking the compatibility! I think this is ready now. +1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants